Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defaulted should clone default value if it's an object #1248

Closed
wants to merge 1 commit into from

Conversation

yeoffrey
Copy link
Contributor

@yeoffrey yeoffrey commented Jun 23, 2024

When using defaulted with a record, it should copy the default value if its an object. This will fix pass-by-reference bugs with this function.

Closes #1238

@arturmuller
Copy link
Collaborator

Thanks @yeoffrey for the PR — #1238 was resolved by merging in #1151 which adds coercers to record (and tuple), so I am closing this.

Having had some time to think about it, there are are two things that I realised:

  1. The custom class use-case is pretty much irrelevant. Superstruct operates primarily structurally, just like TypeScript. We have {...value} and array.slice() calls all over the place, so if you're using custom classes you're gonna have a bad time no matter what. This should be added to README somewhere.
  2. I think that structs should not touch data that is outside than their responsibility. This means that structuredClone is probably not something we want to ever use, since that makes a deep copy which is outside of its responsibility. I think the way superstruct is architected relies on every struct only every making shallow copies and leaving nested logic to subsequent/nested structs. I think this is a big strength, especially when masking and/or using the type struct, as it means that if there are parts of the source object that are not defined as a struct, they can be entirely skipped during processing. If the source objects are large, this can end up being quite efficient.

@arturmuller arturmuller closed this Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defaulted values are not correctly copied for record type data.
2 participants